Fix kernel async statement telemetry handle - #923

🔵 Low — The owning-handle failure path only discards _async_result_stream_started when await_result() raises. If await_result() succeeds (marker stays set) but the subsequent KernelResultSet.__init__ → arrow_schema() raises and is re-wrapped, the guid remains marked as started. A later retry then takes the attach-by-id (no-op telemetry) branch.

Whether this loses the ExecuteStatementAsync telemetry row depends on when the kernel finalizes it: if finalization happens when await_result() returns, this is harmless (telemetry already committed). If finalization only completes once the result stream is drained, the telemetry is lost on this retry because the owning handle is never reused. The PR's own comments ("first in-process result stream", "clear the claimed marker so a retry can still use the telemetry-bearing owning handle") are ambiguous on this point, and the added test_get_execution_result_owning_handle_failure_can_retry_owning_handle only exercises the await_result()-raises case, not the construct-failure-after-await case. Worth confirming the finalization semantics and, if drain-based, discarding the marker on the construction-failure path too.

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

No safe code change is actionable here, and the deciding fact needs kernel-team judgment. The reviewer's suggested fix — discard _async_result_stream_started on the construction-failure path (await succeeds, then _make_result_set/arrow_schema() raises) — would be unsafe: the owning handle's await_result() is single-shot by design (confirmed by test_get_execution_result_is_re_callable, which deliberately re-attaches by id for the second fetch instead of re-calling owning_handle.await_result()). Discarding the marker would route a retry back through the owning handle and re-invoke await_result() on an already-consumed stream — the exact double-consume the marker prevents. Moreover, once await_result() has returned, the stream is materialized and single-shot, so attach-by-id is the only correct retry path regardless. Whether the rare construct-failure-after-await case actually loses the ExecuteStatementAsync telemetry row depends on kernel-internal finalization timing (finalize-at-return vs finalize-at-drain), which has no observable surface in databricks-sql-python and cannot be verified from this repo or a unit test here. The module's own comments assume return-based finalization (making the concern moot); if drain-based, there is no safe in-process remedy. Flagging for a human to confirm finalization semantics with the kernel team.

🔵 Low — This change trades the previously concurrency-safe behavior (always attach-by-id, where every get_query_state / get_execution_result call created its own server-side handle) for use of a shared owning handle before result streaming is claimed. As the new concurrency note candidly documents, handle.status() (and handle.await_result()) now run outside the lock on the connection-shared owning handle. Note that _async_handles is keyed per-connection, not per-cursor, so a fresh cursor on the same connection that adopts the command id (the resume flow exercised by tests/e2e/test_kernel_backend.py::test_async_result_resumable_from_a_fresh_cursor) will also resolve the owning handle and can race the original submitter's polling on the same underlying kernel handle — not just a hand-rolled two-cursor poll. This is an intentional, documented limitation and the common single-submitter flow is safe, so flagging as low: worth confirming the kernel ExecutedAsyncStatement either tolerates concurrent status() or that same-connection concurrent resume is genuinely out of scope.

I corrected the misleading documentation the reviewer flagged. The docstring for get_query_state now accurately reflects that the owning-handle path is per-connection — any cursor on the submitting connection (including a fresh cursor resuming the id) resolves the same owning handle until result streaming is claimed — consistent with the inline concurrency note. Tests pass (125 in the kernel client file).

Pushed 23b4330.

The pushed commit only rewords the docstring — the owning-handle-under-lock behavior at line 721 is unchanged, and the substantive question I raised (does the kernel ExecutedAsyncStatement actually tolerate concurrent status(), or is same-connection concurrent resume genuinely out of scope?) is neither demonstrated nor answered by clearer prose. Better documentation of a limitation isn't verification that the limitation is safe, so I'm keeping this open at Low for a human to confirm the underlying kernel-handle behavior; no further action is needed if that concurrency is confirmed acceptable.

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

Converged to a needs-human question that no code change here can settle: whether the kernel wheel's ExecutedAsyncStatement.status() is safe under concurrent calls on the shared per-connection owning handle. A human must confirm one of: (a) the kernel handle's status()/await_result() tolerate concurrent invocation, or (b) same-connection concurrent resume (tests/e2e/test_kernel_backend.py::test_async_result_resumable_from_a_fresh_cursor racing the original submitter's polling) is genuinely out of scope. Verifying this requires reading the databricks-sql-kernel wheel internals or running the e2e resume test against a live warehouse — this follow-up job has no live-warehouse credentials, so it can't run here. Rewording the docstring again was already rejected by the reviewer as non-verification, and dropping the shared owning handle would revert this PR's intended async-telemetry-finalization fix, so neither is an appropriate change. Flagging for human confirmation of kernel-handle concurrency behavior.